-
Notifications
You must be signed in to change notification settings - Fork 80
feat: implement CliClient #1642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
bin/miden-cli/tests/cli.rs
Outdated
| // Verify the client is functional by syncing | ||
| let sync_result = client.sync_state().await; | ||
| assert!(sync_result.is_ok(), "Client sync failed: {:?}", sync_result.err()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this doesn't really assert that the test ends up using the local config (although this might not be simple anyway)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same applies for the global one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think unfortuantely there is no good way to test this. The assertions didn't actually verify which config was used.
I've fixed this by looking at the actual store file path that was created.
bin/miden-cli/tests/cli.rs
Outdated
| // Attempt to create a client (should fail) | ||
| let client = miden_client_cli::CliClient::from_system_user_config(DebugMode::Disabled).await; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this not silently initialize the client? Or is that not ideal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely correct, I missed that one. I implemented that now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the implementation to call InitCmd::default().execute() when no config is found, and adjusted the test to verify silent initialization works correctly.
bin/miden-cli/src/lib.rs
Outdated
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| pub async fn from_system_user_config(debug_mode: DebugMode) -> Result<Self, CliError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that would be nice is passing a CliConfig to a client constructor, and then have different constructors/loaders for CliConfig (for example, one for local config, one for global, one that respects the priority); it can also save the TOML automatically on the correct directories. We can still keep this from_system_user_config() as well.
I think this also makes tests better in the sense that you don't have to change the working dir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely agree with your suggestion! However, are you fine if we move this to a separate issue to speed up this PR and the mint command (mint has taken quite long already)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, I addressed this too and refactored the CLI config
bin/miden-cli/tests/cli.rs
Outdated
| // Change to the temp directory to ensure the local config is picked up | ||
| let original_dir = env::current_dir().unwrap(); | ||
| env::set_current_dir(&temp_dir)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is also process-wide which could be problematic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed according with #1642 (comment)
UpdateHey everyone, I made a commit with several changes to address the code feedback from @igamigo . Here's what changed:
|
igamigo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I left some comments, most of which are minor. If you prefer to tackle them separately, let's open an issue for them before merging this.
bin/miden-cli/src/utils.rs
Outdated
| /// | ||
| /// # Deprecated | ||
| /// This function is deprecated in favor of `CliConfig::from_system()` which provides | ||
| /// the same functionality with a cleaner API. | ||
| pub(super) fn load_config_file() -> Result<(CliConfig, PathBuf), CliError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just delete this and use the new functions instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Deleted load_config_file() from utils.rs and replaced all usages with CliConfig::from_system() directly
bin/miden-cli/src/lib.rs
Outdated
| /// | ||
| /// # Returns | ||
| /// | ||
| /// A configured `CliClient` instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: These can be doc-linked if you use [] instead:
| /// A configured `CliClient` instance. | |
| /// A configured [`CliClient`] instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happens throughout the file. It's not critical though so if you prefer the current state that's fine too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Updated all documentation to use proper doc-links like [CliClient], [CliConfig],[CliError],[DebugMode::Enabled], etc. Also added cross-references between related methods.
|
One more small thing: For users to make use of client structs and code, I think we'll want to re-export everything from |
Added |
I think it might be better to do |
|
As mentioned in 0xMiden/miden-faucet#196, we should probably look into changing the target of this branch to |
You are absolutely right! And sorry, I should have probably thought about this myself. Cleaner imports are definitely preferred, made the change! |
…n with CLI config
feat(cli): add silent initialization for CliClient test(cli): refactor tests not to use global resources test(cli): modify from_system_user_config CliClient tests to run serially test(cli): modify from_system_user_config tests to effectively assert global and local config prioritization test(cli): add silent initialization tests for from_system_user_config
feat(cli): add ConfigNotFound error variant for better error handling feat(cli): re-export miden_client as client module docs(cli): use doc-links for type references in documentation fix(cli): update test to call from_system_user_config
2ed494e to
5fb2e93
Compare
|
@igamigo I changed the base to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for changing the base branch.
Unless @mmagician wants to take a look, I think we can merge
mmagician
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I think we can merge, and tackle any potential renamings in a follow-up (if there's consensus) so as to not stall this PR.
| help( | ||
| "Run `{} init` command to create a configuration file.", | ||
| client_binary_name().display() | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is CliConfig::default() still desirable to have? If so, which context would it be used in? If not, let's remove impl Default for CliConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default is required by the Provider trait implementation used with figment (see line 71). The implementation provides sensible defaults for relative paths that get resolved when loading from a directory. I've added a doc comment clarifying this usage.
bin/miden-cli/src/lib.rs
Outdated
| /// Gets a reference to the inner `Client<CliKeyStore>`. | ||
| pub fn inner(&self) -> &miden_client::Client<CliKeyStore> { | ||
| &self.0 | ||
| } | ||
|
|
||
| /// Gets a mutable reference to the inner `Client<CliKeyStore>`. | ||
| pub fn inner_mut(&mut self) -> &mut miden_client::Client<CliKeyStore> { | ||
| &mut self.0 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we have DerefMut & Deref implemented, are these two methods needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Removed inner() and inner_mut() since Deref / DerefMut provide the same functionality. Kept into_inner() since it consumes the wrapper (can't be done via deref).
| mod config; | ||
| pub type CliKeyStore = FilesystemKeyStore; | ||
|
|
||
| /// A Client configured using the CLI's system user configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally find the naming "system user configuration", but also don't have a better alternative to propose right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't love it either but not sure what we could rename it to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally considered alternatives like from_cli_config(), from_environment(), and new().
I think the current name, while verbose, most accurately describes what happens: loading config from the system's user configuration locations (local .miden or global ~/.miden).
Open to suggestions if you have a better alternative!
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| pub fn from_system() -> Result<Self, CliError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly here, from_system() doesn't suggest that this is the to-go recommended constructor.
Here, perhaps default() could be used (also see my other comment re: the default constructor)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about discoverability. The challenge with using default() is that it's already used to create blank configs (in init.rs:167), and having it perform filesystem I/O would be surprising. Since users are expected to go through CliClient::from_system_user_config() rather than CliConfig::from_system() directly, I kept the current naming.
Open to alternatives if you have ideas!
|
@Keinberger could you open an issue for tracking potentially renaming or refactoring the |
chore(cli): clarify Default impl usage for CliConfig
Creates this issue: #1686 |
|
Merging this as the test that times out is being tackled on a separate PR and is unrelated to these changes |
Updates
maintonext: feat: implement CliClient #1642 (comment)Summary
Closes #1639
This PR implements a
from_system_user_config()method forCliClientthat creates a Client instance using the same configuration as the CLI tool (reading from.miden/miden-client.tomlor local config). This enables anyone to programmatically obtain a properly configured CLI client.This change is required for implementing the mint command https://github.com/0xMiden/midenup/issues/128, in particular with the new implementation strategy described in 0xMiden/midenup#130 (comment). The https://github.com/0xMiden/miden-faucet repository needs to create a CLI-configured client instance to consume notes and sync state after making the mint request when using the miden-faucet-client, see this PR description for more details: 0xMiden/miden-faucet#196.
Details
New
CliClientwrapper typeA newtype wrapper
CliClient(Client<CliKeyStore>)has been added tomiden-client-cliwithDerefandDerefMutimplementations. This allows external projects to:CliClient::from_system_user_config()Clientmethods transparently through deref coercionClient<CliKeyStore>when needed viainto_inner(),inner(), orinner_mut()Configuration loading behavior
The
from_system_user_config()method searches for configuration files in order:.miden/miden-client.tomlin the current working directory.miden/miden-client.tomlin the home directoryThe client is initialized with the same settings as the CLI:
DebugModeenum, matchingClientBuilder::in_debug_mode()API)The new
CliClienthas been integrated into the existing CLI logic to ensure that the CliClient is the single source of configuration for the CLI client instance.Usage example
This pattern was originally described by @igamigo in the following comment: 0xMiden/midenup#130 (comment)